fix(BA-2404): Generalize gpu_allocated field with improved SlotName and ResourceSlot typing #6087
+271
−73
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
size:XL
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
TypeError, NameError, and AttributeError indicate logical bugs rather than transient failures. Note: SQLAlchemy's StatementError is also a subclass of TypeError.
achimnol
force-pushed
the
fix/generalize-gpu-allocated-stat-field
branch
from
dbb23e9 to
4aab698
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the SlotName type from a simple NewType to a UserString class with parsing capabilities, and updates ResourceSlot to properly handle string keys and normalize values. The changes improve type safety and consistency in GPU resource allocation tracking across different accelerator types (CUDA devices, shares, MIG variants, and NPUs).
Key Changes
- Converted
SlotNamefromNewTypeto aUserStringclass with lazy parsing and accelerator detection capabilities - Enhanced
ResourceSlotto properly normalize keys to strings and process raw values (int, float, str, Decimal, BinarySize) into Decimal - Updated GPU allocation tracking to use generic accelerator detection instead of hardcoded CUDA-specific slot names
- Added serialization support for
SlotNameand improved JSON encoding for test fixtures
Reviewed Changes
Copilot reviewed 23 out of 23 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| src/ai/backend/common/types.py | Core refactoring: SlotName class implementation and ResourceSlot normalization improvements |
| src/ai/backend/common/msgpack.py | Added msgpack serialization support for SlotName |
| src/ai/backend/common/utils.py | Added debugging utility pprint_with_type for type inspection |
| src/ai/backend/common/resilience/policies/retry.py | Added default non-retryable exceptions to retry policy |
| src/ai/backend/manager/models/resource_usage.py | Updated GPU allocation to use SlotName.is_accelerator() |
| src/ai/backend/manager/repositories/user/repository.py | Updated GPU stats aggregation to sum all accelerator slots |
| src/ai/backend/manager/repositories/group/repository.py | Updated container stats to use generic accelerator detection |
| src/ai/backend/manager/repositories/resource_preset/repository.py | Added str() conversion for dict keys |
| src/ai/backend/manager/models/image.py | Added str() conversion for resource limit keys |
| src/ai/backend/manager/api/etcd.py | Added str() conversion for API response keys |
| src/ai/backend/manager/idle.py | Improved variable naming from val to slot_val/util_val |
| src/ai/backend/manager/sokovan/scheduler/selectors/concentrated.py | Added type hint for sort_key list |
| src/ai/backend/agent/resources.py | Updated to use ResourceSlot type and SlotName class |
| src/ai/backend/agent/alloc_map.py | Added str() conversions for fnmatch operations |
| src/ai/backend/agent/docker/agent.py | Changed from dict unpacking to slots.copy() |
| src/ai/backend/agent/dummy/agent.py | Changed from dict unpacking to slots.copy() |
| src/ai/backend/agent/kubernetes/agent.py | Changed from dict unpacking to slots.copy() |
| src/ai/backend/agent/stage/kernel_lifecycle/docker/resource.py | Changed from dict unpacking to slots.copy() |
| src/ai/backend/accelerator/cuda_open/plugin.py | Added str() conversion for slot_name in metadata |
| tests/manager/conftest.py | Enhanced fixture_json_encoder to handle ResourceSlot and Decimal |
| tests/manager/repositories/keypair_resource_policy/test_keypair_resource_policy.py | Fixed test data to use string values for resource slots |
| tests/common/test_types.py | Added comprehensive tests for SlotName parsing and BinarySize/Decimal conversion |
| changes/2404.fix.md | Added changelog entry |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
HyeockJinKim
reviewed
Nov 5, 2025
Comment on lines
+50
to
+54
| DEFAULT_NON_RETRYABLE_ERRORS: Final = ( | ||
| TypeError, | ||
| NameError, | ||
| AttributeError, | ||
| ) |
There was a problem hiding this comment.
Does this need to be provided publicly?
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
resolves #5926 (BA-2404)
Core changes:
SlotNameto be aUserStringsubclass which lazily parses the slot name format.{device_name}.{major_type}[:{minor_type}]gpu_allocatedcalculation to useSlotName.is_accelerator()method to sum all allocated acceleratorsResourceSlotto useUserDictwith proper generic type argumentsstras the key type, but this should be updated toSlotNamein future work (BA-2628; Migrate Manager API to Pydantic-based Request/Response Models #6173)Warning
When there are multiple different accelerators allocated in a single container or installed in a single agent node, this simple "sum" for
gpu_allocatedmay have a non-sense value when mixing different units and fraction scales. We need some design discussion here.This is kept as a known issue because the previous implementation was same in this sense.
Checklist: (if applicable)
SlotNameparsing implementation📚 Documentation preview 📚: https://sorna--6087.org.readthedocs.build/en/6087/
📚 Documentation preview 📚: https://sorna-ko--6087.org.readthedocs.build/ko/6087/